Add general signal hook but keep pari #181
Conversation
mkoeppe
force-pushed
the
add_general_signal_hook_but_keep_pari
branch
from
acd9d8b to
2c32e71
Compare
mkoeppe
force-pushed
the
add_general_signal_hook_but_keep_pari
branch
from
2c32e71 to
fed466d
Compare
|
Yes, this has already been tested, and the corresponding PR sagemath/cypari2#117 was already merged (.... by you, @dimpase) into cypari2, until someone decided to back it out again without discussion. sagemath/cypari2#130 has the change on the cypari2 side of things again. Here I am using an approach that needs less coordination. Keeping the cypari2-specific code (so that current cypari2 works) and adding a general hook (so that future cypari2 with sagemath/cypari2#130 can work). |
| @@ -400,6 +432,7 @@ static void _sig_on_interrupt_received(void) | |||
| cysigs.sig_on_count = 0; | |||
| cysigs.interrupt_received = 0; | |||
| PARI_SIGINT_pending = 0; | |||
| custom_signal_unblock(); | |||
There was a problem hiding this comment.
I think there is a typo in this line: it should have been
custom_set_pending_signal(0);
instead.
In fact, I was having some issues with cysignals 1.12.2 (some doctest failures having to do with alarms, etc) and changing this seems to sovle them.
@kliem can you confirm this is correct?
@antonio-rojas I think this is the fix we were missing. I'm testing, will do a PR tomorrow. Because of the PARI_SIGINT_pending = 0; line, the typo doesn't cause trouble (i.e., until that line is removed).
To be clear, this is the patch I'm testing on top of 1.12.2:
--- a/src/cysignals/implementation.c
+++ b/src/cysignals/implementation.c
@@ -591,7 +591,7 @@ static void _sig_on_interrupt_received(void)
do_raise_exception(cysigs.interrupt_received);
cysigs.sig_on_count = 0;
cysigs.interrupt_received = 0;
- custom_signal_unblock();
+ custom_set_pending_signal(0);
#if HAVE_SIGPROCMASK
sigprocmask(SIG_SETMASK, &oldset, NULL);
There was a problem hiding this comment.
Doesn't quite work for me. Now instead of crashing, tests hang and timeout after 30 minutes.
There was a problem hiding this comment.
Everything works "fine" for me (still getting occasional random failures, but that's not new).
The only difference between cysignals 1.11.4 and 1.12.2 in this respect is this change. Although with 1.11.4, in effect, it is both custom_set_pending_signal(0); and custom_signal_unblock(); but I don't think the unblock there is correct.
Are you using sagemath/cypari2#167 (included in cypari2 2.2.1)? This could make a difference, maybe. I'm not sure if this code is compiled with legacy_implicit_noexcept, you could check the generated .c file to see if it's "clean" as it should be.
The other patch I'm using on cysignals (already for 1.11.4) is: https://github.com/void-linux/void-packages/blob/master/srcpkgs/python3-cysignals/patches/verify_exc_value.patch
There was a problem hiding this comment.
Are you using sagemath/cypari2#167 (included in cypari2 2.2.1)? This could make a difference, maybe. I'm not sure if this code is compiled with legacy_implicit_noexcept, you could check the generated .c file to see if it's "clean" as it should be.
I'm getting the same with cypari2 2.2.0 (+pari 2.17 patches)
There was a problem hiding this comment.
I've released cypari2 2.2.1 - which supports pari 2.17
There was a problem hiding this comment.
ok, not sure what happened before (possibly some memory issues), but it seems to be working now, except that the random failure I reported in sagemath/sage#38749 (comment) is now permanent (regardless of the used seed)
There was a problem hiding this comment.
ok, not sure what happened before (possibly some memory issues), but it seems to be working now, except that the random failure I reported in sagemath/sage#38749 (comment) is now permanent (regardless of the used seed)
Great! I have a PR almost ready to submit (including a test to trigger this bug with cypari2).
As for the remaining issue, I'm getting the same, and I'm working on it. This appears to be related to #215. The implementation of sig_occurred() seems to be incorrect, in the sense that sig_occurred() will sometimes return an old exception that has been already handled.
There was a problem hiding this comment.
is it a basically a 1-line PR (+ the test)?
There was a problem hiding this comment.
is it a basically a 1-line PR (+ the test)?
Yes, I wanted to test it further. I'm also working on the remaining issue which seems more tricky, but I think I have a reasonable fix. Hopefully this might get rid of all the stupid heisenbugs that have been pestering us.
I was actually trying to push this to GH a moment ago, but it seems there is a major outage right now (git pull/push from/to github, are not working atm).
There was a typo in sagemath#181: when the pari sigint handling was converted to a general mechanism, the line ``` PARI_SIGINT_pending = 0; ``` got translated into ``` custom_signal_unblock(); ``` instead of the correct ``` custom_set_pending_signal(0); ``` This error didn't take effect until sagemath#166 removed the pari sigint handling. This causes some doctest failures in sagemath: ``` src/sage/coding/linear_code.py src/sage/geometry/integral_points.pxi src/sage/rings/integer.pyx src/sage/rings/polynomial/polynomial_element.pyx ``` related to mishandling of AlarmInterrupt. See: https://github.com/sagemath/cysignals/pull/181/files#r1904885037
There was a typo in #181: when the pari sigint handling was converted to a general mechanism, the line ``` PARI_SIGINT_pending = 0; ``` got translated into ``` custom_signal_unblock(); ``` instead of the correct ``` custom_set_pending_signal(0); ``` This error didn't take effect until #166 removed the pari sigint handling. This causes some doctest failures in sagemath: ``` src/sage/coding/linear_code.py src/sage/geometry/integral_points.pxi src/sage/rings/integer.pyx src/sage/rings/polynomial/polynomial_element.pyx ``` related to mishandling of AlarmInterrupt. See: https://github.com/sagemath/cysignals/pull/181/files#r1904885037
Introduces a general hook as proposed in the discussion of sagemath/cypari2#109.
Split out from #166. @kliem @videlec